Agree - I think the only way for SCA to be successful is that we tune the warnings/errors to what we want as a community. For example, I disabled the performance->SpreadOperator warning, because it's awfully convenient, even if it comes at a perf disadvantage. If we had the detekt.yml config in spinnaker-gradle-project, we could keep everything synchronized so there's nothing unexpected project-to-project.

I think the biggest issue I tend to have is with arbitrarily bounded checks (e.g. this method is too long).

Yeah, +1000 to error-prone if we can get it working. We have it running everywhere internally (tuned with the errors we care about, as you suggest, and with a ton of custom rules as well) and it's immensely helpful. I think it won't work most places in Spinnaker since we have to use groovy to compile, but I might be wrong.

@plumpy I think you're correct on the Groovy thing, but it looks like the errorprone gradle plugin can be configured to use the JDK9 compiler for Java source sets (its the workaround for supporting JDK8, don't see why this wouldn't also apply for the Groovy compiler?)

There were a lot of exceptions that fail the build. It doesn't instill a lot of confidence - if you could venture a guess, would you say this may be rules that Google doesn't actively use?

Maybe? What sorts of things? Certainly we don't use @Nullable checking by default, for example.

(But have you seen our codebase? It's full of things that probably should have failed the build 😁)

Google has tons of tools and processes for doing large scale changes across entire monorepo, so you can clean up an entire class of errors before enabling the check. Could you just set everything to disabled and just turn on a handful of warnings (not errors) to start? Might not be worth the effort if that's all we're getting out of it, though...

i.e. for the check about how you shouldn't ever use methods that don't specify a charset (like String#getBytes()) we'd have our tool migrate every caller of that method before enabling that check. Presumably we'd have to do the same thing in Spinnaker, but by hand.

Rerunning from the sca-java branch on kork:

/Users/rzienert/netflix/spinnaker/workspace/kork/kork-exceptions/src/main/java/com/netflix/spinnaker/kork/exceptions/ExceptionSummary.java:117: error: An unhandled exception was thrown by the Error Prone static analysis plugin.
    private String message;
                   ^
     Please report this at https://github.com/google/error-prone/issues/new and include the following:
  
     error-prone version: 2.3.4
     BugPattern: ReferenceEquality
     Stack Trace:
     java.lang.IllegalArgumentException: invalid replacement: [3963, -1) (Objects.equals(null, null))
        at com.google.common.base.Preconditions.checkArgument(Preconditions.java:459)
...

As you can see from the code there's nothing particularly interesting going on there.

Perhaps disabling everything and building the rule sets up by hand would be the only way forward. Certainly, nullaway is probably not something we want to enable from the start because, omg, I died inside watching the flurry of warnings go by. It looks like it'd be handy to have, though.

Oh, I see what you mean. Not exceptions indicating a error-prone check failed, but actually a bug in error-prone. Yeah, that's not great 😒

Here's the Java PR: #451

Will merge with more approvals. Anyone against?

Looks like some of the crashes we're running into are the same ones noted in google/error-prone#1195, though that PR has been open for a year.

Only minor comment is the number of things in the .detekt.yml file...are there defaults we could fall back on and only specify the differences so the config file isn't quite so long?

I copied the default and changed things. There is a config for inheriting defaults so things could be much slimmer. My thought was it'd be harder to know if something slipped in as a new rule... but perhaps auto-inheriting new rules isn't such a bad thing?

Only minor comment is the number of things in the .detekt.yml file...are there defaults we could fall back on and only specify the differences so the config file isn't quite so long?

I copied the default and changed things. There is a config for inheriting defaults so things could be much slimmer. My thought was it'd be harder to know if something slipped in as a new rule... but perhaps auto-inheriting new rules isn't such a bad thing?

That's fair...my main concern was just that it's easier to read the config and understand where we deviated from defaults if we're not explicitly setting everything. But maybe I'm placing too much trust in defaults if I was only planning to read where we've deviated from them 🤷‍♂️ . Either way is fine with me, super excited to have code analysis! 👍

Merging. I've altered the config such that it automatically inherits from detekt's defaults and the config we store in the repo is now just customizations. Much bester.